Enable geom_ribbon() to draw separate lines for upper and lower intervals #3529

yutannihilation · 2019-09-11T12:57:02Z

This PR changes GeomRibbon from a normal polygon to a polygon + two lines; while GeomRibbon is implemented as a polygon, considering that the nature of this geom is to indicate the upper and the lower range along X axis, I believe it's natural that GeomRibbon comes with two lines.

Though this is a breaking change, I expect there's very few users using GeomRibbon as a closed line. But, I'm not fully confident... If this expectation would fail, I'll withdraw this PR and create a new geom.

devtools::load_all("~/Documents/repo/ggplot2")
#> Loading ggplot2

df <- tibble::tibble(
  y = c(0, 0, 0, 0, 0, 1, 1.5, 1.7, 1.8), 
  upper_y = y + 0.2, 
  lower_y = y - 0.2,
  year = -4:4
)

# can divide the ribbon
df[4, "upper_y"] <- NA 
df[4, "lower_y"] <- NA

ggplot(df, aes(year, y)) + geom_line() + 
  geom_ribbon(aes(ymax = upper_y, ymin = lower_y),
              linetype = 2, fill = "red", alpha = 0.1, colour = "black")

^{Created on 2019-09-02 by the reprex package (v0.3.0)}

…on-line

thomasp85

LGTM, but @hadley should chime in on breaking changes

R/geom-ribbon.r

yutannihilation · 2019-10-01T09:53:20Z

Thanks! I'm wondering how this breaking change should be described on NEWS. I'll wait for Hadley's comment.

hadley · 2019-10-01T11:38:32Z

Can you explain how this would break existing code? I think the main change is that it won't draw vertical lines on either end, right?

thomasp85 · 2019-10-01T18:29:08Z

It's visually breaking, not something that makes any old code throw an error... I think we had a couple of those for the 3.2.0 release as well. But the NEWS will get rewritten come release so don't worry to much about it right now

hadley · 2019-10-01T20:02:51Z

Yeah, I wouldn't call this a breaking change then.

thomasp85 · 2019-10-02T10:10:20Z

It just occurred to me that this is related to and will influence request for only drawing upper line in geom_area(). As geom_area() subclasses geom_ribbon() it could/should be solved here as well. I suggest adding a parameter to GeomRibbon to control whether upper or both lines are drawn.

We should make sure that the correct line is drawn in geom_area if it dips or is fully below 0

yutannihilation · 2019-10-07T14:27:17Z

Yeah, I wouldn't call this a breaking change then.

Thanks, I got it.

I suggest adding a parameter to GeomRibbon to control whether upper or both lines are drawn.

Thanks, I'll consider this. But, I didn't think about the impact on GeomArea carefully, let me check...

yutannihilation · 2019-10-08T11:34:45Z

Hmm... after some thinking, now I'm not sure if this is OK for geom_area(). How do you feel?

devtools::load_all("~/Documents/repo/ggplot2")
#> Loading ggplot2

huron <- base::data.frame(year = 1875:1972, level = as.vector(LakeHuron))
h <- ggplot(huron, aes(year))
h + geom_area(aes(y = level), alpha = 0.5, colour = "black", size = 2)

^{Created on 2019-10-08 by the reprex package (v0.3.0)}

We probably need these 3 types of Geoms here.

GeomRibbon with separate lines on the upper and the lower
GeomArea with a closed outline (as-is)
GeomArea with separate lines on the upper and the lower

thomasp85 · 2019-10-09T08:36:41Z

I think geom_area() should only have the upper line drawn. This is how most people envision it and is a common request. You can potentially make the behaviour a optional so the old full stroking is accessible.

I don't think anyone wants area plots with upper and lower

yutannihilation · 2019-10-13T07:16:17Z

I don't think anyone wants area plots with upper and lower

After some thinking, now I agree with you here.

I think there are some options to implement this. For example:

upper_colour and lower_colour, which overrides colour for the upper line and the lower line individually (c.f. Allow select alpha to be applied on fill (default), colour, or both #3485 (comment))
Add an option to choose how the line should be drawn (e.g. "both", "upper", "lower", or "polygon").

I'll use the option 2 because it might be good to provide an option to fall back to the previous behavour (polygon).

…on-line

yutannihilation · 2019-10-16T03:20:59Z

We should make sure that the correct line is drawn in geom_area if it dips or is fully below 0

It seems I need to add some tweaks on geom_area(), though I'm yet to figure out why these differ.

devtools::load_all("~/Documents/repo/ggplot2/")
#> Loading ggplot2

d <- base::data.frame(x = 1:10, y = 5 - 1:10)

p1 <- ggplot(d, aes(x, ymin = 0, ymax = y)) +
  geom_ribbon(alpha = 0.3, colour = "red", size = 3, outline.type = "upper")

p2 <- ggplot(d, aes(x, y)) +
  geom_area(alpha = 0.3, colour = "red", size = 3, outline.type = "upper")

patchwork::wrap_plots(p1, p2)

^{Created on 2019-10-16 by the reprex package (v0.3.0)}

yutannihilation · 2019-10-27T08:26:40Z

Probably, I need another version of PositionStack to complete this? I can flip ymin and ymax when ymax is negative in GeomArea$draw_group(), but I feel it should be done on Position's side.

thomasp85 · 2019-11-04T07:53:38Z

This is not the domain of positions IMO... Pretty sure the reason for what you are seeing is that geom_area() has a pmin()/pmax() call on the y range somewhere before plotting. Maybe we can just remove that?

yutannihilation · 2019-11-04T09:35:17Z

geom_area() has a pmin()/pmax() call on the y range somewhere before plotting

I too thought so at first, but GeomArea does nothing special at the drawing stage.

ggplot2/R/geom-ribbon.r

Lines 170 to 187 in 40e8b60

    
           GeomArea <- ggproto("GeomArea", GeomRibbon, 
        
             default_aes = aes(colour = NA, fill = "grey20", size = 0.5, linetype = 1, 
        
               alpha = NA), 
        
             required_aes = c("x", "y"), 
        
             setup_params = function(data, params) { 
        
               params$flipped_aes <- has_flipped_aes(data, params, ambiguous = TRUE) 
        
               params 
        
             }, 
        
             setup_data = function(data, params) { 
        
               data$flipped_aes <- params$flipped_aes 
        
               data <- flip_data(data, params$flipped_aes) 
        
               data <- transform(data[order(data$PANEL, data$group, data$x), ], ymin = 0, ymax = y) 
        
               flip_data(data, params$flipped_aes) 
        
             } 
        
           )

I think the call of pmax() and pmin() in question is done in pos_stack(), which is used in PositionStack$compute_panel().

ggplot2/R/position-stack.r

Lines 220 to 221 in 40e8b60

    
           df$ymin <- pmin(heights[-n], heights[-1]) 
        
           df$ymax <- pmax(heights[-n], heights[-1])

thomasp85 · 2019-11-04T09:37:36Z

damn, you are probably right... that is inconvenient

Maybe position_stack should keep track of which values it flip and switch them back in the end

thomasp85 · 2019-12-16T12:50:43Z

I've created a PR that addresses the observed problems with geom_area

I think we should have a switch for the new behaviour, both in geom_ribbon and geom_area... there will be some breaking visual changes and giving people a way to opt out will be good. I think the new behaviour should be default, though. Am I right in that the current implementation doesn't allow for old-style full stroking?

thomasp85 · 2019-12-17T14:08:32Z

@yutannihilation can you check that everything is as it should be after the merge of #3673? If so I think we can finish this off

R/geom-ribbon.r

Co-Authored-By: Thomas Lin Pedersen <thomasp85@gmail.com>

yutannihilation · 2019-12-18T03:25:41Z

Thanks, confirmed the problem is now fixed after #3673.

devtools::load_all("~/Documents/repo/ggplot2/")
#> Loading ggplot2

d <- base::data.frame(x = 1:10, y = 5 - 1:10)

p1 <- ggplot(d, aes(x, ymin = 0, ymax = y)) +
  geom_ribbon(alpha = 0.3, colour = "red", size = 3, outline.type = "upper")

p2 <- ggplot(d, aes(x, y)) +
  geom_area(alpha = 0.3, colour = "red", size = 3, outline.type = "upper")

patchwork::wrap_plots(p1, p2)

^{Created on 2019-12-18 by the reprex package (v0.3.0)}

I think we should have a switch for the new behaviour, both in geom_ribbon and geom_area... there will be some breaking visual changes and giving people a way to opt out will be good. I think the new behaviour should be default, though. Am I right in that the current implementation doesn't allow for old-style full stroking?

Sorry, I missed this comment. Yes, I agree with you and the users can specify "legacy" option for the old-style full stroking. I'll add a NEWS item to describe the change briefly. Do I need to add some examples for this?

ggplot2/R/geom-ribbon.r

Lines 25 to 27 in 7c6d3d5

    
           #' @param outline.type Type of the outline of the area; `"both"` draws both the 
        
           #'   upper and lower lines, `"upper"` draws the upper lines only. `"legacy"` 
        
           #'   draws a closed polygon around the area.

…on-line

yutannihilation · 2020-01-04T13:26:33Z

I'm merging this. Thanks for reviewing multiple times!

yutannihilation added 4 commits September 2, 2019 16:47

Make geom_ribbon() draw lines separately from polygons

017992b

Merge remote-tracking branch 'upstream/master' into feature/geom-ribb…

5973b2c

…on-line

Update a visual case

43d12b1

Fix a test expectation

b871f0d

thomasp85 approved these changes Oct 1, 2019

View reviewed changes

thomasp85 added this to the ggplot2 3.3.0 milestone Oct 1, 2019

yutannihilation commented Oct 1, 2019

View reviewed changes

R/geom-ribbon.r Outdated Show resolved Hide resolved

Fix a typo

2b7b26c

yutannihilation added 7 commits October 13, 2019 17:42

Add option outline.type

1cab6a8

Merge remote-tracking branch 'upstream/master' into feature/geom-ribb…

e38b438

…on-line

Fix argument of geom_ribbon() and geom_area() to match with GeomRibbon

1a5c225

Add a test for outline.type

f6f67e2

Remove whitespaces

0cff7cd

Ignore argument checks on geom_area()

9c21ed7

Fix test

69a762d

yutannihilation mentioned this pull request Nov 4, 2019

Allow PositionStack to specify the directions on negatives and positives seperately #3607

Closed

thomasp85 mentioned this pull request Dec 16, 2019

Don't switch relationship of ymin/ymax during stacking #3673

Merged

thomasp85 reviewed Dec 17, 2019

View reviewed changes

R/geom-ribbon.r Outdated Show resolved Hide resolved

yutannihilation and others added 2 commits December 18, 2019 12:09

Update R/geom-ribbon.r

7d8554a

Co-Authored-By: Thomas Lin Pedersen <thomasp85@gmail.com>

Merge branch 'master' into feature/geom-ribbon-line

7c6d3d5

yutannihilation added 6 commits December 19, 2019 20:26

Merge branch 'master' into feature/geom-ribbon-line

eb07f59

Add a NEWS bullet

5f2c106

Remove suspicious diff

f761f3a

Warn on outline = "legacy"

6c485e2

Merge remote-tracking branch 'upstream/master' into feature/geom-ribb…

a668795

…on-line

Update a visual case

d827fd3

yutannihilation merged commit a8acd82 into tidyverse:master Jan 4, 2020

yutannihilation deleted the feature/geom-ribbon-line branch January 4, 2020 14:46

yutannihilation mentioned this pull request Jan 8, 2020

Set outline type for geom_density as geom_area #3708

Merged

yutannihilation mentioned this pull request May 24, 2020

munched_lines warning from geom_area() #4023

Closed

corybrunson mentioned this pull request Oct 6, 2022

don't draw vertical lines at ends of alluvia and flows corybrunson/ggalluvial#104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable geom_ribbon() to draw separate lines for upper and lower intervals #3529

Enable geom_ribbon() to draw separate lines for upper and lower intervals #3529

yutannihilation commented Sep 11, 2019

thomasp85 left a comment

yutannihilation commented Oct 1, 2019

hadley commented Oct 1, 2019

thomasp85 commented Oct 1, 2019

hadley commented Oct 1, 2019

thomasp85 commented Oct 2, 2019

yutannihilation commented Oct 7, 2019

yutannihilation commented Oct 8, 2019

thomasp85 commented Oct 9, 2019

yutannihilation commented Oct 13, 2019

yutannihilation commented Oct 16, 2019

yutannihilation commented Oct 27, 2019

thomasp85 commented Nov 4, 2019

yutannihilation commented Nov 4, 2019

thomasp85 commented Nov 4, 2019

thomasp85 commented Dec 16, 2019 •

edited

Loading

thomasp85 commented Dec 17, 2019

yutannihilation commented Dec 18, 2019

yutannihilation commented Jan 4, 2020

Enable geom_ribbon() to draw separate lines for upper and lower intervals #3529

Enable geom_ribbon() to draw separate lines for upper and lower intervals #3529

Conversation

yutannihilation commented Sep 11, 2019

thomasp85 left a comment

Choose a reason for hiding this comment

yutannihilation commented Oct 1, 2019

hadley commented Oct 1, 2019

thomasp85 commented Oct 1, 2019

hadley commented Oct 1, 2019

thomasp85 commented Oct 2, 2019

yutannihilation commented Oct 7, 2019

yutannihilation commented Oct 8, 2019

thomasp85 commented Oct 9, 2019

yutannihilation commented Oct 13, 2019

yutannihilation commented Oct 16, 2019

yutannihilation commented Oct 27, 2019

thomasp85 commented Nov 4, 2019

yutannihilation commented Nov 4, 2019

thomasp85 commented Nov 4, 2019

thomasp85 commented Dec 16, 2019 • edited Loading

thomasp85 commented Dec 17, 2019

yutannihilation commented Dec 18, 2019

yutannihilation commented Jan 4, 2020

thomasp85 commented Dec 16, 2019 •

edited

Loading